Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix @spawn_or_run_task with interactive threads #3385

Merged
merged 3 commits into from
Oct 10, 2023
Merged

Fix @spawn_or_run_task with interactive threads #3385

merged 3 commits into from
Oct 10, 2023

Conversation

nalimilan
Copy link
Member

When Julia ≥ 1.9 is started with -tX,Y, by default tasks use the pool of interactive threads instead of the default pool. Fix this by updating spawn_or_run_task to match the code used by @spawn in Julia master.

Fixes #3383.

When Julia ≥ 1.9 is started with `-tX,Y`, by default tasks use the pool
of interactive threads instead of the default pool. Fix this by updating
`spawn_or_run_task` to match the code used by `@spawn` in Julia master.
@nalimilan nalimilan requested a review from bkamins October 6, 2023 20:54
@bkamins
Copy link
Member

bkamins commented Oct 7, 2023

Do we also need to update macro spawn_or_run(threads, expr)?

@bkamins
Copy link
Member

bkamins commented Oct 7, 2023

And also maybe we should add to documentation that we spawn threads in :default mode?

@nalimilan
Copy link
Member Author

What do you think about setting JULIA_NUM_THREADS=4,1 for tests? Interestingly, this appears to be accepted by Julia 1.6.5, giving 4 threads (it seems anything after the digit is ignored).

@bkamins
Copy link
Member

bkamins commented Oct 7, 2023

What do you think about setting JULIA_NUM_THREADS=4,1 for tests?

OK

@bkamins bkamins added the ecosystem Issues in DataFrames.jl ecosystem label Oct 7, 2023
@bkamins bkamins added this to the 1.7 milestone Oct 7, 2023
@bkamins
Copy link
Member

bkamins commented Oct 9, 2023

Looks good. Do you think we should additionally ask someone more who knows threading internals for review?

@nalimilan
Copy link
Member Author

Given that I just copied @spawn I think we're safe. ;-)

@bkamins
Copy link
Member

bkamins commented Oct 9, 2023

OK - so I think it is safe to merge this once you are ready :).
Maybe one small thing - can you also please add a NEWS.md entry for this?

@nalimilan nalimilan merged commit 87c2162 into main Oct 10, 2023
3 of 7 checks passed
@nalimilan nalimilan deleted the nl/spawn branch October 10, 2023 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ecosystem Issues in DataFrames.jl ecosystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests of describe and multithreading fail in Julia-1.10.0-beta3
2 participants